[flang] Don't duplicate impure function call for UBOUND() #153648
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because the per-dimension information in a descriptor holds an extent and a lower bound, but not an upper bound, the calculation of the upper bound sometimes requires that the extent and lower bound be extracted from a descriptor and added together, minus 1. This shouldn't be attempted when the NamedEntity of the descriptor is something that shouldn't be duplicated and used twice; specifically, it shouldn't apply to NamedEntities containing references to impure functions as parts of subscript expressions. Fixes llvm#153031.
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-semantics Author: Peter Klausler (klausler) ChangesBecause the per-dimension information in a descriptor holds an extent and a lower bound, but not an upper bound, the calculation of the upper bound sometimes requires that the extent and lower bound be extracted from a descriptor and added together, minus 1. This shouldn't be attempted when the NamedEntity of the descriptor is something that shouldn't be duplicated and used twice; specifically, it shouldn't apply to NamedEntities containing references to impure functions as parts of subscript expressions. Fixes #153031. Full diff: https://github.com/llvm/llvm-project/pull/153648.diff 3 Files Affected:
diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index 212356136d6ee..74c6acbcb1ed5 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -1144,15 +1144,14 @@ std::optional<std::string> FindImpureCall(
std::optional<std::string> FindImpureCall(
FoldingContext &, const ProcedureRef &);
-// Predicate: is a scalar expression suitable for naive scalar expansion
-// in the flattening of an array expression?
-// TODO: capture such scalar expansions in temporaries, flatten everything
-class UnexpandabilityFindingVisitor
- : public AnyTraverse<UnexpandabilityFindingVisitor> {
+// Predicate: does an expression contain anything that would prevent it from
+// being duplicated so that two instances of it then appear in the same
+// expression?
+class UnsafeToCopyVisitor : public AnyTraverse<UnsafeToCopyVisitor> {
public:
- using Base = AnyTraverse<UnexpandabilityFindingVisitor>;
+ using Base = AnyTraverse<UnsafeToCopyVisitor>;
using Base::operator();
- explicit UnexpandabilityFindingVisitor(bool admitPureCall)
+ explicit UnsafeToCopyVisitor(bool admitPureCall)
: Base{*this}, admitPureCall_{admitPureCall} {}
template <typename T> bool operator()(const FunctionRef<T> &procRef) {
return !admitPureCall_ || !procRef.proc().IsPure();
@@ -1163,14 +1162,22 @@ class UnexpandabilityFindingVisitor
bool admitPureCall_{false};
};
+template <typename A>
+bool IsSafelyCopyable(const A &x, bool admitPureCall = false) {
+ return !UnsafeToCopyVisitor{admitPureCall}(x);
+}
+
+// Predicate: is a scalar expression suitable for naive scalar expansion
+// in the flattening of an array expression?
+// TODO: capture such scalar expansions in temporaries, flatten everything
template <typename T>
bool IsExpandableScalar(const Expr<T> &expr, FoldingContext &context,
const Shape &shape, bool admitPureCall = false) {
- if (UnexpandabilityFindingVisitor{admitPureCall}(expr)) {
+ if (IsSafelyCopyable(expr, admitPureCall)) {
+ return true;
+ } else {
auto extents{AsConstantExtents(context, shape)};
return extents && !HasNegativeExtent(*extents) && GetSize(*extents) == 1;
- } else {
- return true;
}
}
diff --git a/flang/lib/Evaluate/shape.cpp b/flang/lib/Evaluate/shape.cpp
index 776866d1416d2..894049f32a6bf 100644
--- a/flang/lib/Evaluate/shape.cpp
+++ b/flang/lib/Evaluate/shape.cpp
@@ -623,7 +623,7 @@ MaybeExtentExpr GetRawUpperBound(
} else if (semantics::IsAssumedSizeArray(symbol) &&
dimension + 1 == symbol.Rank()) {
return std::nullopt;
- } else {
+ } else if (IsSafelyCopyable(base, /*admitPureCall=*/true)) {
return ComputeUpperBound(
GetRawLowerBound(base, dimension), GetExtent(base, dimension));
}
@@ -678,9 +678,11 @@ static MaybeExtentExpr GetUBOUND(FoldingContext *context,
} else if (semantics::IsAssumedSizeArray(symbol) &&
dimension + 1 == symbol.Rank()) {
return std::nullopt; // UBOUND() folding replaces with -1
- } else if (auto lb{GetLBOUND(base, dimension, invariantOnly)}) {
- return ComputeUpperBound(
- std::move(*lb), GetExtent(base, dimension, invariantOnly));
+ } else if (IsSafelyCopyable(base, /*admitPureCall=*/true)) {
+ if (auto lb{GetLBOUND(base, dimension, invariantOnly)}) {
+ return ComputeUpperBound(
+ std::move(*lb), GetExtent(base, dimension, invariantOnly));
+ }
}
}
} else if (const auto *assoc{
diff --git a/flang/test/Evaluate/bug153031.f90 b/flang/test/Evaluate/bug153031.f90
new file mode 100644
index 0000000000000..a717954ecaed1
--- /dev/null
+++ b/flang/test/Evaluate/bug153031.f90
@@ -0,0 +1,18 @@
+! RUN: %flang_fc1 -fdebug-unparse %s 2>&1 | FileCheck %s
+! Ensure that UBOUND() calculation from LBOUND()+SIZE() isn't applied to
+! variables containing references to impure functions.
+type t
+ real, allocatable :: a(:)
+end type
+interface
+ pure integer function pure(n)
+ integer, intent(in) :: n
+ end
+end interface
+type(t) :: x(10)
+allocate(x(1)%a(2))
+!CHECK: PRINT *, ubound(x(int(impure(1_4),kind=8))%a,dim=1_4)
+print *, ubound(x(impure(1))%a, dim=1)
+!CHECK: PRINT *, int(size(x(int(pure(1_4),kind=8))%a,dim=1,kind=8)+lbound(x(int(pure(1_4),kind=8))%a,dim=1,kind=8)-1_8,kind=4)
+print *, ubound(x(pure(1))%a, dim=1)
+end
|
| allocate(x(1)%a(2)) | ||
| !CHECK: PRINT *, ubound(x(int(impure(1_4),kind=8))%a,dim=1_4) | ||
| print *, ubound(x(impure(1))%a, dim=1) | ||
| !CHECK: PRINT *, int(size(x(int(pure(1_4),kind=8))%a,dim=1,kind=8)+lbound(x(int(pure(1_4),kind=8))%a,dim=1,kind=8)-1_8,kind=4) |
Contributor
There was a problem hiding this comment.
Ok, so because it's PURE, it's ok to call it multiple times with the same value, because it's supposed to produce the same results on the same inputs. (And there's no way in Fortran to check how many times it's called without making it impure :-) )
Contributor
There was a problem hiding this comment.
Should Flang also pull out the evaluation of the PURE function to get better runtime performance even though it has no side-effect being evaluated multiple times?
Contributor
Author
There was a problem hiding this comment.
CSE can do so in optimization if it wants.
eugeneepshteyn
approved these changes
Aug 14, 2025
DanielCChen
approved these changes
Aug 14, 2025
Contributor
DanielCChen
left a comment
DanielCChen
left a comment
There was a problem hiding this comment.
LGTM.
It fixed the full test case too.
Thanks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because the per-dimension information in a descriptor holds an extent and a lower bound, but not an upper bound, the calculation of the upper bound sometimes requires that the extent and lower bound be extracted from a descriptor and added together, minus 1. This shouldn't be attempted when the NamedEntity of the descriptor is something that shouldn't be duplicated and used twice; specifically, it shouldn't apply to NamedEntities containing references to impure functions as parts of subscript expressions.
Fixes #153031.